Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release/7.0] Fix Settings_MaxHeaderListSize_Server #46251

Closed
wants to merge 2 commits into from
Closed

[release/7.0] Fix Settings_MaxHeaderListSize_Server #46251

wants to merge 2 commits into from

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 24, 2023

Ports test fix for #45811 which is already in main. Can wait for March.

@wtgodbe wtgodbe requested a review from a team January 24, 2023 18:20
@ghost ghost added the area-runtime label Jan 24, 2023
@ghost ghost added this to the 7.0.x milestone Jan 24, 2023
@ghost
Copy link

ghost commented Jan 24, 2023

Hi @wtgodbe. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@wtgodbe wtgodbe modified the milestones: 7.0.x, 7.0.4 Jan 24, 2023
@wtgodbe wtgodbe added the tell-mode Indicates a PR which is being merged during tell-mode label Jan 24, 2023
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 24, 2023

Hmm, looks like this fix is incomplete. Gonna see if I missed something

Assert.Throws() Failure
Expected: typeof(System.Net.Http.HttpRequestException)
Actual: (No exception was thrown)

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 24, 2023

I don't see anything in the file history in main that isn't here, @Tratcher any idea what's missing?

@Tratcher
Copy link
Member

Tratcher commented Jan 24, 2023 •
edited
Loading

Can you check the response status code (the prior condition)? Are you sure you have the runtime change available in this branch?

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 24, 2023

Can you check the response status code (the prior condition)?

Which one do you mean? I don't see us checking that in the test

Are you sure you have the runtime change available in this branch?

I couldn't find which runtime change caused the original failure, it wasn't linked in the PR - is it this one? dotnet/runtime#79281 - that is in 7.0. And we are currently seeing the same test failure as described in #45811 in 7.0

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 24, 2023

Oh, I know what's happening - internal dependency flow. That change probably hasn't flowed to the public branch yet. I bet this'll work after the internal -> public merge.

@ghost
Copy link

ghost commented Feb 7, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 7, 2023
@dougbu dougbu removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 46251 in repo dotnet/aspnetcore

@dougbu
Copy link
Member

dougbu commented Feb 7, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@wtgodbe
Copy link
Member Author

wtgodbe commented Feb 7, 2023

I suspect this won't work until next Tuesday (internal -> public merge that'll bring in the dependency flow commits from Feb). If that messes with the timing we could just move this PR to the internal repo - @mmitche what's your preference?

@dougbu dougbu enabled auto-merge (squash) February 7, 2023 22:39
@wtgodbe wtgodbe mentioned this pull request Feb 14, 2023
Merged
@dougbu
Copy link
Member

dougbu commented Feb 15, 2023

Merged commits from here into #46646. Suggest closing.

@wtgodbe wtgodbe closed this Feb 15, 2023
auto-merge was automatically disabled February 15, 2023 01:50

Pull request was closed

@wtgodbe wtgodbe deleted the wtgodbe/FixTest branch February 15, 2023 01:51
@eerhardt eerhardt mentioned this pull request Mar 7, 2023
Merged
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@wtgodbe wtgodbe restored the wtgodbe/FixTest branch November 15, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@halter73 halter73 Awaiting requested review from halter73

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@JamesNK JamesNK Awaiting requested review from JamesNK

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
7.0.4
Development

Successfully merging this pull request may close these issues.
5 participants
@wtgodbe @Tratcher @dougbu @amcasey @captainsafia